Skip to content

Improve efficiency of new_dynamic by catching IdenticalZero and IdenticalOne cases #232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

perrydv
Copy link

@perrydv perrydv commented Jun 5, 2025

This PR should improve efficiency of new_dynamic. Many of the basic math ops such as add, add_eq, sub, sub_eq, mul, mul_eq, div, div_eq, and azmul catch cases where one argument is a CppAD Constant and has value either 0 or 1, when this allows avoiding placing an operation on the AD tape. For example y = x + c where c is identically 0 does not require an addition op to be placed on the tape.

However, such checking was not in place for cases where neither argument is a CppAD Variable but rather at least one of them is a CppAD Dynamic. This meant that the operation tape used for updating dynamics (via new_dynamic) could include potentially many trivial operations such as adding 0 or multiplying by 1. In a simple example such as double-taping of second derivatives of a quadratic form such as in a multivariate normal density calculation (e.g. x' A x, where A is dynamic and derivatives are with respect to x) could generate a mostly wasteful new_dynamic tape.

This PR attempts to imitate the logic used for the Variable cases to handle similar cases for Dynamic. In my test problem, it fixes a memory explosion that was occurring. All the ops listed above were modified.

This PR is likely not ready to merge because I am not sure how to add to CppAD's test suite. @bradbell is that something you could do, or provide guidance on? I am not clear if there is comprehensive testing of Dynamic operations or other similar place that might be obvious to extend. My own motivating problem only touches a couple of these ops, so the others remain untested. I am making this PR now to see if this looks on the right track to you before spending more time on it.

I note that pow could also receive the same kind of extension as the other ops listed above. However, I noticed that for the Variable cases, it checks for if( IdenticalZero( y.value_ ) ) and if( IdenticalZero( x.value_ ) ), so it is not checking if y or x are constants the way the code for the other ops does. E.g. in the other ops, there would be something like if( (!dyn_y) && IdenticalZero( y.value_ ) ). Any chance this is a bug for pow?

Also @bradbell you told me to be sure I could run bin/check_all.sh. That has been pretty hard to get fully working. I believe all the code checks run, and make check (called from build) passes. But bin/check_all.sh gets stuck in a documentation step and it's been a bit of an obstacle.

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2025

CLA assistant check
All committers have signed the CLA.

@bradbell
Copy link
Contributor

bradbell commented Jun 7, 2025

So far so good. I loaded all the optional packages (so they get tested) using bin/get_optional.sh and executed

bin/check_all.sh --skip_external_links

and to the final response bin/check_all.sh: OK . I skipped the documentation external link check because something seems be happening on the gnu.org site.

@bradbell
Copy link
Contributor

bradbell commented Jun 7, 2025

We need a test that fails before the change and passes after. The only change is in the number of dynamic parameters in the tape. See the following for an example of how this gets tested:
https://github.com/coin-or/CppAD/blob/master/test_more/general/optimize.cpp

Search for size_var() in the file and you will see places that check that the number of variables gets reduced. I think that you will need to use size_dyn_par(); see
https://cppad.readthedocs.io/latest/fun_property.html

One thing though. This is not an optimizer change because it is done during the recording. So size_dyn_par() will be smaller (even before you optimize). Maybe a good place to add this test is in the file
https://github.com/coin-or/CppAD/blob/master/test_more/general/parameter.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants